Enable runtime closure verification - #41698
Conversation
|
Tagging subscribers to this area: @safern, @ViktorHofer |
NikolaMilosavljevic
added
area-Infrastructure-installer
and removed
area-Infrastructure-libraries
labels
| </Target> | ||
|
|
||
| <!-- | ||
| Overriding VerifyClosure target to enable closure validation without other validations. |
There was a problem hiding this comment.
If this is just temporary rather than forking the whole thing, just have a single target:
<Target Name="EnsureDepenedencyGraphDir" BeforeTargets="VerifyClosure">
<MakeDir Directories="$(PackageReportDir)$(Id)" />
</Target>There was a problem hiding this comment.
I actually need to override the target to update it's condition as well, to run if new property is set - ShouldVerifyClosure. Currently VerifyClosure only runs if SkipValidatePackage is false: https://github.com/dotnet/arcade/blob/e11b8486ad155fab13e788a268755c0ca60d3609/src/Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk/targets/framework.packaging.targets#L170
There was a problem hiding this comment.
Is there a reason we can’t get the arcade update instead? Would it take too long? Could you manually update just the package with this fix?
There was a problem hiding this comment.
I've heard that there is a freeze on whole Arcade update, but I'll ask if a single package would be allowed.
There was a problem hiding this comment.
Heard back - we can get update in RC2. I will update VerifyClosure target in Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk in Arcade, and remove the target override from my PR. But, I will pick your suggestion for a new target to create Reports directory, which works around an issue in VerifyClosure task: dotnet/arcade#6090. The task is in a different Arcade package and they want to minimize the number of changes now.
There was a problem hiding this comment.
@ericstj I will also move your suggested new target into Arcade next to VerifyClosure target. We can, then, remove this target when VerifyClosure task is fixed - with a single PR in Arcade.
There was a problem hiding this comment.
Will keep EnsureDepenedencyGraphDir in runtime repo - PackageReportDir is runtime-specific property, it doesn't make sense for this to be in Arcade.
There was a problem hiding this comment.
Arcade fix is in: dotnet/arcade#6101
This (runtime) fix can be merged and closure validation would start working once we pick up new Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk package.
@ericstj @jkoritzinsky this is safe to merge once you approve the changes.
|
If I understand correctly the current state of this PR is not running the target and you need to include the new package in order to fix the condition. Is that correct? |
Yes, that's correct - this PR needs to be completed by 9/4, but Arcade change will come a bit later. |
@ericstj I don't mind copying If we do this, we ensure that target runs and we're not blocked by package update. |
|
I see, one thing I've done in the past is manually update a single package once signed build is available (which is typically an hour or two after check-in). Is that not possible? |
Yeah - I've added that change now. |
ericstj
left a comment
ericstj
left a comment
There was a problem hiding this comment.
This looks good. Did you confirm the target is running?
Yes - confirmed in my local build, but will check PR validation build as well, once it completes. |
Yes, the target is running. However, I might need to update this PR. Double-checking right now, but I might need to pick a new version of another package, due to some refactoring done about a month ago, by @mmitche : dotnet/arcade@4864664#diff-af7c096fa15e403c04f3343d8a820d57 |
Confirmed that the target is running in validation builds. Also confirmed that dependent intra-Arcade package is properly restored during build. |
|
/backport to release/5.0-rc2 |
|
Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/238245693 |
|
@NikolaMilosavljevic backporting to release/5.0-rc2 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Enable closure validation
Applying: Ensure dependency graph directory exists
Applying: Netstandard should not be ignored
Applying: Ensure Reports dir exists; VerifyClosure change moved to Arcade
Applying: Upgrade SharedFramework.SDK package to pick up a change for VerifyClosure target
error: sha1 information is lacking or useless (eng/Version.Details.xml).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Upgrade SharedFramework.SDK package to pick up a change for VerifyClosure target
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
ghost
locked as resolved and limited conversation to collaborators
4 participants
Fixes: #31805
Overriding Arcade target to allow closure verification without other validations and to allow us to incorporate a fix for a bug in VerifyClosure. As discovered during fix verification, the task would fail if a destination folder (for DGML file) does not exist - issue in Arcade: dotnet/arcade#6090